Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the old style multithreading #10056

Merged
merged 10 commits into from
Nov 6, 2017
Merged

Remove the old style multithreading #10056

merged 10 commits into from
Nov 6, 2017

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Nov 5, 2017

Running the display list interpreter asynchronously is too dangerous, as has been discussed, and at least for Vulkan we now have a much better alternative, which will soon be turned on by default.

The old stuff does work for some games and there might be some people out there relying on it, but the benefit was never huge and the cost in code complexity and synchronization wasn't worth it.

So bye bye, "Multithreading (experimental)". Vulkan with the new multithreading outperforms the other backends so much it's not even funny sometimes.

Following fixes are by making them obsolete. The new multithreading implementation for Vulkan and the planned one for GL will not have these issues.

Fixes #9712
Fixes #8895
Fixes #7877
Fixes #7774
Fixes #7494
Fixes #6874
Fixes #6612
Fixes #6441
Fixes #6331
Fixes #6096
Fixes #5989
Fixes #5632
Fixes #5127

Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cathartic. Thanks.

-[Unknown]

@@ -392,10 +392,10 @@ void System_Wake() {
// Ugly!
static bool pspIsInited = false;
static bool pspIsIniting = false;
static bool pspIsQuiting = false;
static bool pspIsQuitting = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derp, this is my typo. Thanks.

-[Unknown]

@@ -193,13 +193,12 @@ static void __GeExecuteInterrupt(u64 userdata, int cyclesLate) {
__TriggerInterrupt(PSP_INTR_IMMEDIATE, PSP_GE_INTR, PSP_INTR_SUB_NONE);
}

// Should we still do this?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - even if we wanted to try to sync CPU cycles with GPU cycles, I think we'd need to do it very differently. I think __GeCheckCycles can be removed entirely.

-[Unknown]

@unknownbrackets
Copy link
Collaborator

We can also move CoreTiming::ScheduleEvent_Threadsafe to CoreTiming::ScheduleEvent (same with UnscheduleThreadsafeEvent) in sceGe.cpp, and remove the nearby comments in sceGe. I can PR later though.

-[Unknown]

@hrydgard
Copy link
Owner Author

hrydgard commented Nov 5, 2017

I'll just go ahead and do it. Gonna do a little more testing too later before I merge this.

There are, by the way, no other active users of threadsafe events anywhere, except for some commented-out code in adhoc. But I think I'll keep around the functions for now.

@hrydgard hrydgard force-pushed the remove-multithreading-2 branch from 953d041 to a827ad2 Compare November 5, 2017 23:28
@zminhquanz
Copy link
Contributor

zminhquanz commented Nov 6, 2017

How about for the old device does not support Vulkan API ? what are you doing with this for without multithreading

@unknownbrackets
Copy link
Collaborator

We may eventually try to apply the Vulkan threading approach to GLES as well.

The current multithreading is very buggy, and causes a lot of glitches and crashes. Worse, it depends on GPU/driver speed/other factors for what bugs will happen, meaning it can seem to work for some or in some scenes of the game, and then break for unexpected reasons elsewhere.

-[Unknown]

@NotUnknownbrackets
Copy link

may

@LunaMoo
Copy link
Collaborator

LunaMoo commented Nov 6, 2017

Removing MT will fix all of those and lots more unreported cases where people keep it enabled ignoring the warnings or had it activated all the time from before the warnings were added and breaking their experience.
Few games being able to work faster on some outdated device or accidently improving other issues is not worth the amount of hardware dependant BS this option currently causes.

Seeing some trolling/account created for the sake of being against this PR just hours after it was opened, I think it'll be healthy to limit the discussion here. Removal of existing options in any emulator has always people against it, but in the long run, it's necessary.

Repository owner locked and limited conversation to collaborators Nov 6, 2017
@hrydgard hrydgard merged commit ea2fc55 into master Nov 6, 2017
@hrydgard hrydgard deleted the remove-multithreading-2 branch November 6, 2017 19:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.